Skip to content

Conversation

@gpotter2
Copy link
Member

@gpotter2 gpotter2 commented Nov 26, 2025

This is a pretty big PR that changes many aspects related to the cryptography of Windows protocols. This is removes the Chain helper class, which was pretty much useless and replaces it with better alternatives. This PR:

  • add CMS signing / check
  • refactor (a lot) scapy/layers/tls/cert.py add add documentation
    • Chain wasn't as useful as it could be. We now have a CertTree class that serves as a certificate store (think like Windows), and properly links certificates against their issuer, to a list of root CAs
    • getchain() allows to find a chain of certificates towards one
    • verify() says if a certificate can be verified against the store
  • support PKINIT in Kerberos
  • Load KRB5CCNAME in SPNEGO.from_cli_arguments
  • much better support of X509_AlgorithmIdentifier
  • fix Kerberos handling in DCE/RPC passive sniffing
  • improve Kerberos handling of the "DELEGATION"-related structures
  • KerberosSSP now supports simply having a TGT, and can also be created from Ticketer using ssp()
  • Improve DCE/RPC context handling (keeps the context IDs when re-binding to the same interface, etc.)
  • Add support for Kerberos secure channels in MS-NRPC
  • Add the ability to chose between WindowsNT, Windows2000 and Windows2003+ variants of NTLM when building packets
  • add an ability to NOT send channel bindings in HTTP
  • add doc regarding some existing Kerberos features (e.g. FAST)

@codecov
Copy link

codecov bot commented Nov 26, 2025

Codecov Report

❌ Patch coverage is 59.09879% with 236 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.79%. Comparing base (e73137e) to head (7ea2540).

Files with missing lines Patch % Lines
scapy/layers/kerberos.py 21.98% 110 Missing ⚠️
scapy/layers/tls/cert.py 77.88% 48 Missing ⚠️
scapy/layers/msrpce/msnrpc.py 7.31% 38 Missing ⚠️
scapy/layers/msrpce/rpcclient.py 50.00% 24 Missing ⚠️
scapy/layers/ntlm.py 70.00% 12 Missing ⚠️
scapy/libs/rfc3961.py 81.81% 2 Missing ⚠️
scapy/asn1fields.py 88.88% 1 Missing ⚠️
scapy/layers/spnego.py 96.15% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4879      +/-   ##
==========================================
- Coverage   80.86%   80.79%   -0.07%     
==========================================
  Files         368      368              
  Lines       90271    90565     +294     
==========================================
+ Hits        72996    73174     +178     
- Misses      17275    17391     +116     
Files with missing lines Coverage Δ
scapy/asn1/mib.py 91.66% <100.00%> (+0.05%) ⬆️
scapy/layers/dcerpc.py 90.10% <100.00%> (+0.04%) ⬆️
scapy/layers/http.py 83.68% <100.00%> (+0.02%) ⬆️
scapy/layers/smb.py 76.70% <ø> (-0.36%) ⬇️
scapy/layers/smbclient.py 73.05% <ø> (ø)
scapy/layers/x509.py 97.95% <100.00%> (+0.39%) ⬆️
scapy/modules/ticketer.py 41.56% <100.00%> (+0.41%) ⬆️
scapy/asn1fields.py 83.43% <88.88%> (-0.28%) ⬇️
scapy/layers/spnego.py 78.86% <96.15%> (+8.06%) ⬆️
scapy/libs/rfc3961.py 85.80% <81.81%> (-0.07%) ⬇️
... and 5 more

... and 6 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@gpotter2 gpotter2 force-pushed the begin-pkinit branch 2 times, most recently from e7169e7 to c583fe1 Compare November 26, 2025 19:05
@gpotter2 gpotter2 changed the title Windows: add Kerberos PKINIT, Netlogon's Kerberos secure channel, better NTLM variants Crypto rework: CertTree, Kerberos PKINIT, Netlogon's Kerberos secure channel, better NTLM variants Dec 12, 2025
@gpotter2 gpotter2 changed the title Crypto rework: CertTree, Kerberos PKINIT, Netlogon's Kerberos secure channel, better NTLM variants Windows/Crypto rework: CertTree, Kerberos PKINIT, Netlogon's Kerberos secure channel, better NTLM variants Dec 12, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR significantly refactors Scapy's Windows cryptography infrastructure with major improvements to certificate handling, Kerberos PKINIT support, and DCE/RPC functionality.

Key Changes:

  • Replaces the Chain helper class with a more sophisticated CertTree class that properly organizes certificates in a tree structure with ROOT CA verification
  • Implements PKINIT (Public Key Cryptography for Initial Authentication in Kerberos) with Diffie-Hellman key exchange
  • Adds support for loading Kerberos credentials from KRB5CCNAME environment variable and ccache files
  • Introduces NTLM variant support (WindowsNT, Windows2000, Windows2003+) for backward compatibility
  • Implements CMS (Cryptographic Message Syntax) signing and verification
  • Adds Kerberos secure channel support in MS-NRPC (Netlogon)
  • Improves DCE/RPC context management and binding

Reviewed changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
scapy/layers/tls/cert.py Major refactor: adds CertTree, CertList, CMS_Engine classes; improves certificate verification
scapy/layers/x509.py Enhanced X509_AlgorithmIdentifier with proper RFC compliance and MultipleTypeField handling
scapy/layers/kerberos.py PKINIT implementation, octetstring2key, enhanced KerberosSSP with TGT support
scapy/layers/ntlm.py NTLM_VARIANT system for supporting different Windows versions
scapy/layers/spnego.py KRB5CCNAME support, ccache loading, improved from_cli_arguments
scapy/modules/ticketer.py Enhanced ssp() method to distinguish TGT/ST, added iter_tickets()
scapy/layers/msrpce/rpcclient.py Better context management, endpoint resolution, impersonation level support
scapy/layers/msrpce/msnrpc.py Kerberos secure channel in Netlogon
scapy/libs/rfc3961.py RFC4556 octetstring2key implementation
test files Comprehensive test coverage for new features
Comments suppressed due to low confidence (2)

scapy/layers/smbclient.py:695

            use_ioctl=kwargs.pop("use_ioctl", True),

scapy/layers/smbclient.py:696

            timeout=kwargs.pop("timeout", 3),

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +542 to +549
OFFSET = lambda pkt: (
32
if (
pkt.VARIANT == NTLM_VARIANT.NT_OR_2000
or (pkt.DomainNameBufferOffset or 40) <= 32
)
else 40
)
Copy link

Copilot AI Dec 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The OFFSET lambda expressions are overly complex and difficult to read. Consider extracting this logic into a helper method or breaking it into multiple simpler conditional expressions for better maintainability.

Suggested change
OFFSET = lambda pkt: (
32
if (
pkt.VARIANT == NTLM_VARIANT.NT_OR_2000
or (pkt.DomainNameBufferOffset or 40) <= 32
)
else 40
)
@staticmethod
def get_offset(pkt):
"""
Helper method to compute the offset for NTLM_NEGOTIATE.
"""
if (
pkt.VARIANT == NTLM_VARIANT.NT_OR_2000
or (pkt.DomainNameBufferOffset or 40) <= 32
):
return 32
else:
return 40
OFFSET = get_offset

Copilot uses AI. Check for mistakes.
Comment on lines +3533 to +3534
# XXX UNFINISHED
raise NotImplementedError
Copy link

Copilot AI Dec 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment on line 3534 says "XXX UNFINISHED" and raises NotImplementedError for PUBLIC_KEY method in PKINIT. This incomplete implementation should either be completed or documented as a limitation in the PR description.

Suggested change
# XXX UNFINISHED
raise NotImplementedError
# FIXME: PKINIT Public Key Encryption (RFC4556 section 3.2.3.2) is not yet implemented in Scapy.
# This is a known limitation. See https://github.com/secdev/scapy/issues/ for progress.
raise NotImplementedError(
"PKINIT Public Key Encryption (RFC4556 section 3.2.3.2) is not yet implemented in Scapy. "
"If you require this feature, please open an issue or contribute an implementation."
)

Copilot uses AI. Check for mistakes.
Comment on lines +5186 to +5202
# authenticator_checksum.Deleg = KRB_CRED(
# tickets=[self.TGT],
# encPart=EncryptedData()
# )
# authenticator_checksum.encPart.encrypt(
# Context.STSessionKey,
# EncKrbCredPart(
# ticketInfo=KrbCredInfo(
# key=EncryptionKey.fromKey(self.TGTSessionKey),
# prealm=ASN1_GENERAL_STRING(crealm),
# pname=PrincipalName.fromUPN(self.UPN),
# # TODO: rework API to pass starttime... here.
# sreralm=self.TGT.realm,
# sname=self.TGT.sname,
# )
# )
# )
Copy link

Copilot AI Dec 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The NotImplementedError for GSS_C_DELEG_FLAG contains commented-out implementation code (lines 5186-5202). Either implement this feature or remove the commented code to avoid confusion.

Suggested change
# authenticator_checksum.Deleg = KRB_CRED(
# tickets=[self.TGT],
# encPart=EncryptedData()
# )
# authenticator_checksum.encPart.encrypt(
# Context.STSessionKey,
# EncKrbCredPart(
# ticketInfo=KrbCredInfo(
# key=EncryptionKey.fromKey(self.TGTSessionKey),
# prealm=ASN1_GENERAL_STRING(crealm),
# pname=PrincipalName.fromUPN(self.UPN),
# # TODO: rework API to pass starttime... here.
# sreralm=self.TGT.realm,
# sname=self.TGT.sname,
# )
# )
# )

Copilot uses AI. Check for mistakes.

on:
pull_request:
push:
Copy link

Copilot AI Dec 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The workflow trigger has been changed from 'pull_request' to 'push', which means CIFuzz will run on every push to master rather than on PRs. This could increase CI usage and costs. Verify this is intentional.

Suggested change
push:
pull_request:

Copilot uses AI. Check for mistakes.
Comment on lines +4657 to +4658
import scapy.libs.rfc3961 # Trigger error if any # noqa: F401

Copy link

Copilot AI Dec 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Module 'scapy.libs.rfc3961' is imported with both 'import' and 'import from'.

Suggested change
import scapy.libs.rfc3961 # Trigger error if any # noqa: F401

Copilot uses AI. Check for mistakes.
####################


class CertList(list):
Copy link

Copilot AI Dec 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The class 'CertList' does not override 'eq', but adds the new attribute frmt.
The class 'CertList' does not override 'eq', but adds the new attribute frmt.
The class 'CertList' does not override 'eq', but adds the new attribute frmt.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant